Fix bug where -backend-config could not override attributes that weren't at the top level in the backend schema
#36919
Conversation
| @@ -48,19 +50,57 @@ func Reset() { | |||
|
|
|||
| // New creates a new backend for Inmem remote state. | |||
| func New() backend.Backend { | |||
| if os.Getenv("TF_INMEM_TEST") != "" { | |||
There was a problem hiding this comment.
A choice that I needed to consider here was either to:
- Make the
inmembackend have test-specific schemas. - Or, add a new in-memory backend that was only available for use in the context of testing.
Currently the inmem backend is available for end users to use if they know it exists (it is not in any docs), but it was intended as a test resource. Making the inmem backend unavailable to end users and only available during tests is a breaking change. We could do that in future, but for now I think making parts of the inmem backend test-only makes sense, instead of creating a new backend that uses 99% of the same code.
There was a problem hiding this comment.
I think it'd be great to have this comment somewhere in this code for our future selves.
| case isNested: | ||
| // The flag item is overriding a nested attribute | ||
| // e.g. assume_role.role_arn in the s3 backend | ||
| // We assume a max nesting-depth of 1 as s3 is the only |
There was a problem hiding this comment.
Can we somehow bake this assumption in the code validation?
An input like "test_nesting_single.child.grand" was allowed to pass with no error, and the child value ended up being set.
|
|
||
| parentName := splitName[0] | ||
| nestedName := splitName[1] | ||
| parentAttr := schema.Attributes[parentName] |
There was a problem hiding this comment.
There is a path to panicking here, if the schema does not have parentName
e.g "-backend-config=test_nesting_single_not_exists.child=foobar"
| if os.Getenv("TF_INMEM_TEST") != "" { | ||
| // We use a different schema for testing. This isn't user facing unless they | ||
| // dig into the code. | ||
| fmt.Sprintln("TF_INMEM_TEST is set: Using test schema for the inmem backend") |
There was a problem hiding this comment.
This is using Sprintln, which returns a string, as opposed to printing to the CLI. If that was intentional, then this can simply be a comment.
| @@ -48,19 +50,57 @@ func Reset() { | |||
|
|
|||
| // New creates a new backend for Inmem remote state. | |||
| func New() backend.Backend { | |||
| if os.Getenv("TF_INMEM_TEST") != "" { | |||
There was a problem hiding this comment.
I think it'd be great to have this comment somewhere in this code for our future selves.
|
Thanks for the review @dsa0x! I think I'll pivot to a different approach so that this doesn't have that assumption about how many levels of nesting are present. But I'm learning more about navigating cty.Values in the process so it'll take a little time |
Fixes #36911
The bug reported in that issue is due to how the code that processes
-backend-configflags assumes that any key=value pairs passed in are setting values for a top-level attribute in the backend's schema. The error shared in the fixed GH issue arises due to the code looking for a top-level field with the nameassume_role.role_arninstead of looking forrole_arnnested withinassume_role.terraform/internal/command/init.go
Lines 1036 to 1044 in 474fe47
To enable testing of this update I needed access to a backend with a schema that matches the s3 backend's use of single nesting. Instead of adding a test that uses the s3 backend and needing to supply credentials, I've updated the inmem backend to optionally have an expanded backend for testing purposes. This is controlled by an environment variable specific to that backend. This could be a first step towards considering making the whole inmem backend not user facing and used 100% for tests.
Target Release
1.13.x
CHANGELOG entry